Require posture on strand creation#544
Open
flyingrobots wants to merge 7 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthrough
ChangesRetentionPosture enforcement across strand lifecycle
Sequence Diagram(s)sequenceDiagram
participant Client
participant ForkStrandRequest
participant WorldlineRuntime
participant StrandRegistry
participant SettlementService
participant BraidShellAssembly
Client->>ForkStrandRequest: from_session_default(session_ctx)
ForkStrandRequest->>ForkStrandRequest: retention_posture = session_ctx.retention_posture()
ForkStrandRequest-->>Client: ForkStrandRequest { retention_posture }
Client->>WorldlineRuntime: fork_strand(request)
WorldlineRuntime->>StrandRegistry: insert(Strand { retention_posture })
StrandRegistry->>StrandRegistry: retention_posture.validate()
alt posture is incoherent
StrandRegistry-->>WorldlineRuntime: StrandError::Posture(PostureObstruction)
else posture is valid
StrandRegistry-->>WorldlineRuntime: Ok
WorldlineRuntime-->>Client: ForkStrandReceipt { retention_posture }
end
Client->>SettlementService: plan_with_policy(strand)
SettlementService->>SettlementService: check causal_posture == Shared
alt posture is not Shared
SettlementService-->>Client: SettlementError::NonSharedStrand
else posture is Shared
SettlementService->>SettlementService: plural_draft(strand_posture)
SettlementService-->>Client: SettlementPlan
end
Client->>SettlementService: settle_with_policy(plan)
SettlementService->>BraidShellAssembly: build_braid_shell(strand_posture)
BraidShellAssembly->>BraidShellAssembly: BraidShellMember.posture := strand_posture
BraidShellAssembly->>BraidShellAssembly: BraidShell::assemble(strand_posture)
BraidShellAssembly-->>SettlementService: BraidShell
SettlementService-->>Client: SettlementResult { braid_shell }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eacd3876d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
@codex please confirm these are the complete actionable issues for this pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eacd3876d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@codex Self-discovered issue during P1 warp-wasm RED verification.
|
|
Summary
Testing
|
Owner
Author
Activity Summary@codex Review repair pass completed for the posture-on-strand-creation PR.
Resolved review threads: Local witnesses:
Pre-push witnesses passed on final head |
Summary
Testing / Checks
|
Owner
Author
|
@codex please confirm this self-review finding. Self-Code Review Findings
No P0/P1 findings were found in this self-review. Remote CI is green at the time of review, and the worktree was clean before fetch/review. |
Owner
Author
|
Resolved the self-review P2 in What changed:
Local witnesses:
@codex self-review finding is addressed; please confirm. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 426-431: The SettlementError::NonSharedStrand case in the
warp_kernel.rs file is incorrectly using the error_codes::INVALID_STRAND code,
which is documented as "requested strand is not registered" in the ABI contract.
However, this case represents a different error condition where the strand
exists but settlement is forbidden by its posture policy. Either create a new
dedicated error code constant for settlement-policy rejection and use it in the
NonSharedStrand match arm, or coordinate with the team to intentionally broaden
the ABI contract definition for code 14 and update all downstream callers and
documentation accordingly. Do not leave the meaning of the error code ambiguous
on the wire.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61466ee0-086f-4459-b6c1-611aa718e135
📒 Files selected for processing (10)
CHANGELOG.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/neighborhood.rscrates/warp-core/src/observation.rscrates/warp-core/src/revelation.rscrates/warp-core/src/settlement.rscrates/warp-core/src/strand.rscrates/warp-core/tests/strand_contract_tests.rscrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rs
Comment on lines
+426
to
+431
| SettlementError::NonSharedStrand { strand_id, posture } => AbiError { | ||
| code: error_codes::INVALID_STRAND, | ||
| message: format!( | ||
| "strand {strand_id:?} with posture {posture:?} is not shared-admitted for settlement" | ||
| ), | ||
| }, |
Contributor
There was a problem hiding this comment.
Don’t overload INVALID_STRAND for admission-policy rejection.
INVALID_STRAND is still documented as “requested strand is not registered” in the ABI contract. Reusing code 14 for NonSharedStrand changes the machine-readable meaning on the wire and makes “strand missing” indistinguishable from “strand exists but settlement is forbidden by posture policy”. Either add a dedicated ABI code for settlement-policy rejection, or update the ABI/spec contract and downstream callers in the same change if code 14 is being broadened intentionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/warp-wasm/src/warp_kernel.rs` around lines 426 - 431, The
SettlementError::NonSharedStrand case in the warp_kernel.rs file is incorrectly
using the error_codes::INVALID_STRAND code, which is documented as "requested
strand is not registered" in the ABI contract. However, this case represents a
different error condition where the strand exists but settlement is forbidden by
its posture policy. Either create a new dedicated error code constant for
settlement-policy rejection and use it in the NonSharedStrand match arm, or
coordinate with the team to intentionally broaden the ABI contract definition
for code 14 and update all downstream callers and documentation accordingly. Do
not leave the meaning of the error code ambiguous on the wire.
Owner
Author
|
@codex please confirm this self-review finding. Self-Code Review Findings
No additional findings were identified in this pass. Pre-review lock passed: clean worktree, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c7ca74ef2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+697
to
+701
| if strand.retention_posture.causal_posture != CausalPosture::Shared { | ||
| return Err(SettlementError::NonSharedStrand { | ||
| strand_id, | ||
| posture: strand.retention_posture.causal_posture, | ||
| }); |
There was a problem hiding this comment.
When a strand is AuthorOnly or Scratch, this new guard rejects only plan/settle; the public WASM path compare_settlement still calls SettlementService::compare directly (crates/warp-wasm/src/warp_kernel.rs:1091-1098) and returns a SettlementDelta containing the source worldline, source entries, and basis report. Because this commit makes debugger/non-shared strands carry AuthorOnly posture, any caller that can name such a strand can still inspect suffix evidence that the new settlement invariant says is not shared-admitted, so the same posture check should live in compare or at that ABI boundary before any delta is built.
Useful? React with 👍 / 👎.
Owner
Author
|
Follow-up correction after discussion: the earlier P1 finding over-prescribed the mitigation. The doctrine we want is:
Locked this into
Local witnesses passed:
@codex please treat the earlier P1 comment as superseded by this clarified invariant. |
|
Summary
Testing
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backlog/cool-ideas/CI-003-append-only-braid-membership.md`:
- Around line 48-52: The AuthorOnly bullet point is ambiguous about the contract
for unauthorized weaving attempts—it does not clarify whether an unauthorized
weave is rejected outright or admitted but recorded as a sealed reference.
Rewrite the AuthorOnly bullet to explicitly separate these two distinct paths:
clearly state what happens when weaving with authority (the allowed case) and
what happens when weaving without authority (the rejection or
sealed-audit-record case), so implementers understand these are different
guarantees and do not conflate a sealed audit record with actual membership.
In `@CHANGELOG.md`:
- Around line 554-559: The CHANGELOG entry uses the phrase "locally held strand
suffix" which is too broad and could be misinterpreted as referring to any
locally present strand. Replace "locally held strand suffix" with "held under
local authority" to more precisely reflect the compare doctrine's actual
boundary and clarify that the compare-vs-plan/settle split is stricter than the
current wording implies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3fc5ce36-a0bc-4f49-bbe8-cfc7f3c35ca1
📒 Files selected for processing (4)
CHANGELOG.mdbacklog/cool-ideas/CI-003-append-only-braid-membership.mdcrates/warp-core/src/settlement.rscrates/warp-wasm/src/warp_kernel.rs
Comment on lines
+48
to
+52
| - A shared braid projection can reveal the relationship without revealing a | ||
| sealed member source chain. | ||
| - Settlement can admit a braid projection without collapsing member strands. | ||
| - Weaving in an AuthorOnly member requires authority or records a sealed | ||
| member reference. |
Contributor
There was a problem hiding this comment.
Separate allowed weaving from sealed audit records.
The AuthorOnly bullet is ambiguous about whether an unauthorized weave is admitted or rejected and merely recorded. Those are different contracts; spell out the rejection path explicitly so implementers don’t treat a sealed reference as membership.
Suggested wording
- - Weaving in an AuthorOnly member requires authority or records a sealed
- member reference.
+ - Weaving in an AuthorOnly member requires authority.
+ - Unauthorized weave attempts are rejected; any sealed member reference is
+ audit-only and does not change membership.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - A shared braid projection can reveal the relationship without revealing a | |
| sealed member source chain. | |
| - Settlement can admit a braid projection without collapsing member strands. | |
| - Weaving in an AuthorOnly member requires authority or records a sealed | |
| member reference. | |
| - A shared braid projection can reveal the relationship without revealing a | |
| sealed member source chain. | |
| - Settlement can admit a braid projection without collapsing member strands. | |
| - Weaving in an AuthorOnly member requires authority. | |
| - Unauthorized weave attempts are rejected; any sealed member reference is | |
| audit-only and does not change membership. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backlog/cool-ideas/CI-003-append-only-braid-membership.md` around lines 48 -
52, The AuthorOnly bullet point is ambiguous about the contract for unauthorized
weaving attempts—it does not clarify whether an unauthorized weave is rejected
outright or admitted but recorded as a sealed reference. Rewrite the AuthorOnly
bullet to explicitly separate these two distinct paths: clearly state what
happens when weaving with authority (the allowed case) and what happens when
weaving without authority (the rejection or sealed-audit-record case), so
implementers understand these are different guarantees and do not conflate a
sealed audit record with actual membership.
Comment on lines
+554
to
+559
| - `warp-core` settlement planning now rejects non-`Shared` strands before | ||
| producing import candidates. Author-only/debugger strand suffixes can remain | ||
| real causal work, but they cannot enter base shared history without an | ||
| explicit shared admission posture. Settlement compare remains local | ||
| revelation/inspection only: it can inspect a locally held strand suffix | ||
| without promoting, planning, admitting, or settling it. |
Contributor
There was a problem hiding this comment.
Tighten the compare boundary to local authority.
“Locally held strand suffix” is broader than the finalized compare doctrine and can be read as any locally present strand being inspectable. Say “held under local authority” so this entry doesn’t drift from the compare-vs-plan/settle split.
Suggested wording
- revelation/inspection only: it can inspect a locally held strand suffix
+ revelation/inspection only: it can inspect a strand suffix held under local authority
without promoting, planning, admitting, or settling it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - `warp-core` settlement planning now rejects non-`Shared` strands before | |
| producing import candidates. Author-only/debugger strand suffixes can remain | |
| real causal work, but they cannot enter base shared history without an | |
| explicit shared admission posture. Settlement compare remains local | |
| revelation/inspection only: it can inspect a locally held strand suffix | |
| without promoting, planning, admitting, or settling it. | |
| - `warp-core` settlement planning now rejects non-`Shared` strands before | |
| producing import candidates. Author-only/debugger strand suffixes can remain | |
| real causal work, but they cannot enter base shared history without an | |
| explicit shared admission posture. Settlement compare remains local | |
| revelation/inspection only: it can inspect a strand suffix held under local authority | |
| without promoting, planning, admitting, or settling it. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 554 - 559, The CHANGELOG entry uses the phrase
"locally held strand suffix" which is too broad and could be misinterpreted as
referring to any locally present strand. Replace "locally held strand suffix"
with "held under local authority" to more precisely reflect the compare
doctrine's actual boundary and clarify that the compare-vs-plan/settle split is
stricter than the current wording implies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RetentionPosturethroughForkStrandRequest,ForkStrandReceipt, andStrandso strand creation is posture-explicit.AuthorOnlyand never silently enterSharedhistory.StrandRegistry::insertand hash posture obstructions deterministically for scheduler fault evidence.Refs #538.
Validation
cargo check -p warp-corecargo test -p warp-core --libcargo test -p warp-core --test strand_contract_testscargo clippy -p warp-core --lib -- -D warnings -D missing_docscargo fmt --all -- --checkgit diff --checkscripts/check-causal-posture-constructors.shscripts/ban-nondeterminism.shpnpm exec markdownlint-cli2 CHANGELOG.mdSummary by CodeRabbit
Release Notes
New Features
Changed
Bug Fixes
INVALID_STRANDerror code (with posture details).